-
Notifications
You must be signed in to change notification settings - Fork 438
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pharokka wrapper #5130
Pharokka wrapper #5130
Conversation
tools/pharokka/pharokka.xml
Outdated
<!-- check file size since output is non-deterministic --> | ||
<output name="pharokka_gbk"> | ||
<assert_contents> | ||
<has_size value="353875" delta="300" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
has_size is the worst test you can do as it is very unspecific, you could use other more specific asserts if you like
tools/pharokka/pharokka.xml
Outdated
#else: | ||
echo "use cache" && | ||
mkdir pharokka_db && | ||
tar -xvf "$reference_source.db_cached.fields.path" --strip 1 -C pharokka_db && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use everywhere single-quotes, see here: https://galaxy-iuc-standards.readthedocs.io/en/latest/best_practices/tool_xml.html#command-formatting
tools/pharokka/pharokka.xml
Outdated
|
||
## run tool | ||
#if str( $terminase.terminase_selector ) == "no_terminase": | ||
pharokka.py -i $fasta -o pharokka_output -d pharokka_db -t 8 $gene_predictor $meta -e $evalue && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
single-quote all data params and text params
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is -t 8 the number of cores? if so please use GALAXY_SLOTS
tools/pharokka/pharokka.xml
Outdated
## create output | ||
zip -r out.zip pharokka_db && | ||
cp out.zip "$archive_output" && | ||
cp pharokka_output/pharokka.gbk "$pharokka_gbk" && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of copy, you can use from_work_dir=
in the output section
tools/pharokka/pharokka.xml
Outdated
</command> | ||
<inputs> | ||
<!-- the genome --> | ||
<param type="data" name="fasta" format="data" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a title here and maybe some help to assist the user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With title, do you mean using a label tag ?
tools/pharokka/pharokka.xml
Outdated
</param> | ||
</when> | ||
<when value="history"> | ||
<param name="db_histroy" type="data" format="data" label="Use the folloing pharokka DB" help="You can upload a pharokka DB as tar.gz to the history and use it as DB" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
format is wrong, or needs to be specified
* improved tests * added archive test
* single quotes changed * zip as test data * improved tests * GALAXY_SLOTS * using from_work_dir=
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- please see if you can add a bio.tools ID
tools/pharokka/pharokka.xml
Outdated
rapid standardised annotation tool for bacteriophage genomes and metagenomes | ||
</description> | ||
<requirements> | ||
<requirement type='package' version='1.2.0'> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use macos and a TOKEN here, see https://galaxy-iuc-standards.readthedocs.io/en/latest/best_practices/tool_xml.html#tool-versions
tools/pharokka/pharokka.xml
Outdated
|
||
## run tool | ||
#if str( $terminase.terminase_selector ) == 'no_terminase': | ||
pharokka.py -i $fasta -o pharokka_output -d pharokka_db -t \${GALAXY_SLOTS:-8} $gene_predictor $meta -e $evalue && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all paths and text parameter needs to be single-quoted
tools/pharokka/pharokka.xml
Outdated
]]> | ||
</help> | ||
<citations> | ||
<citation type='bibtex'> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use type=doi here
tools/pharokka/pharokka.xml
Outdated
</param> | ||
</when> | ||
<when value='history'> | ||
<param name='db_histroy' type='data' format='zip' label='Use the folloing pharokka DB' help='You can upload a pharokka DB as zip to the history and use it as DB' /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where do users get such a DB?
tools/pharokka/pharokka.xml
Outdated
</conditional> | ||
</inputs> | ||
<outputs> | ||
<data name='archive_output' format='zip' from_work_dir='out.zip' label='${tool.name} on ${on_string}: zip of the complete output' /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make this output file optional, so that the user needs to select an option to output it ... I don't think its so useful by default.
* optional zip output * DB source * single-quotes in cheetah * citation doi * macros and tokens * bio tools ID
…s-iuc into pharokka-wrapper
* added test DB as folder
tools/pharokka/pharokka.xml
Outdated
<![CDATA[ | ||
pharokka is a rapid standardised annotation tool for bacteriophage genomes and metagenomes. | ||
|
||
If you are looking for rapid standardised annotation of bacterial genomes, please use prokka, which inspired the creation of pharokka, or bakta. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bakta should be used today, so I guess its save to recommend bakta.
This help could be enhanced a bit and more information be given?
tools/pharokka/pharokka.xml
Outdated
</option> | ||
</param> | ||
<param name="meta" type="boolean" checked="false" truevalue="--meta" falsevalue="" label="meta mode for metavirome input samples" /> | ||
<param name="evalue" type="integer" value="100000" label="E-value threshold for mmseqs2 PHROGs database search. Defaults to 1E-05." /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use min and max value whenever you can for floats and ints. the value looks for an evalue strange.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it, but the values go from 1e-20 to 10, however the bar does not really allow choosing something like 1e-10 ... I think it would be a good feature if a log scale could be used...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But max 10 you can add correct?
tools/pharokka/pharokka.xml
Outdated
## create output | ||
#if $zip_output == 'true': | ||
zip -r out.zip pharokka_output | ||
#else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this else should not be needed
<command detect_errors="exit_code"> | ||
<![CDATA[ | ||
## run tool | ||
#if str( $terminase.terminase_selector ) == 'no_terminase': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't there a else case missing for run_terminase?
tools/pharokka/.shed.yml
Outdated
long_description: | | ||
pharokka is a rapid standardised annotation tool for bacteriophage genomes and metagenomes. | ||
If you are looking for rapid standardised annotation of bacterial genomes, please use prokka, | ||
which inspired the creation of pharokka, or bakta. Repository-Maintainer: Paul Zierep |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can get credits by adding tags to the tools ... see https://docs.galaxyproject.org/en/latest/dev/schema.html
And add yourself as maintainer via the codeowner file in this repo.
tools/pharokka/pharokka.xml
Outdated
@@ -0,0 +1,154 @@ | |||
<tool id="pharokka" name="bacteriophage annotation" version="@TOOL_VERSION@+galaxy@VERSION_SUFFIX@" python_template_version="3.7" profile="@PROFILE@"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove the python_template_version its not needed.
]]> | ||
</help> | ||
<citations> | ||
<citation type="bibtex"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use a doi style citation https://docs.galaxyproject.org/en/latest/dev/schema.html#tool-citations-citation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was addressed, github for some reason does not show it here.
Thanks @paulzierep |
Attention: deployment skipped! https://github.com/galaxyproject/tools-iuc/actions/runs/4308502357 |
Due to test failures. Will restart .. lets see |
Attention: deployment skipped! https://github.com/galaxyproject/tools-iuc/actions/runs/4308502357 |
Still fails with exit code 1 |
So it seems that in biopython Could you do this @paulzierep? .. maybe also ask the pharokka developers why they use an extra gff module .. biopython itself should have a gff parser |
For the record, the error in bcbio-gff is tracked there (there's a pending PR) |
I guess that can be solved by updating to v1.2.1 gbouras13/pharokka#238 |
Think so. Could you prepare a PR for the IUC repo? Note, I also added the biopython pin to bcbio-gff: bioconda/bioconda-recipes#39703 |
Ahh. you already did :) Still wondering why our CI did not catch this problem before merging ... |
FOR CONTRIBUTOR:
Had initial problems with testing the data table. But the maintainer provided a tiny version of the DB and the tests passed. Tests are only written for the main output.